Add set_ticket_key_callback (SSL_CTX_set_tlsext_ticket_key_cb) #330
Conversation
toidiu
force-pushed
the
toidiu/ticket_key_callback
branch
from
5a54a4d to
28f7678
Compare
Add a wrapper for the `SSL_CTX_set_tlsext_ticket_key_cb`, which allows consumers to configure the EVP_CIPHER_CTX and HMAC_CTX used for encrypting/decrypting session tickets. See https://docs.openssl.org/1.0.2/man3/SSL_CTX_set_tlsext_ticket_key_cb/ for more details.
toidiu
force-pushed
the
toidiu/ticket_key_callback
branch
from
28f7678 to
9a6e166
Compare
| F: Fn( | ||
| &SslRef, | ||
| &mut [u8; 16], | ||
| *mut u8, |
There was a problem hiding this comment.
The IV is constrained to a max length (https://github.com/google/boringssl/blob/main/ssl/ssl_session.cc#L331-L335) so instead of giving applications a raw pointer we should handle the unsafe conversion to a slice constrained to ffi::EVP_MAX_IV_LENGTH ourselves
There was a problem hiding this comment.
Yea good call. I wasn't sure if the length was dependent on the cipher.
There was a problem hiding this comment.
When encrypting a new ticket, encrypt will be one. It writes a public 16-byte key name to key_name and a fresh IV to iv.
Just to confirm, are we ok looking at BoringSSL implementation details to derive this API? The docs, quoted above only guarantee a 16 byte key name and a fresh IV (unspecified length).
There was a problem hiding this comment.
The key_name and iv are uninitialized, so they can't be u8 arrays/slices! If they had to be slices, then &mut [MaybeUninit<u8>; 16] would be appropriate.
The iv array can use boring_sys::EVP_MAX_IV_LENGTH for the length.
We can't force the callback initialize those fields. I think that makes it unsafe in Rust terms, because safe Rust code could leave them as-is, report success, and make boring read uninit bytes.
The Rust wrapper of this callback needs to first clear these fields (MaybeUninit would work, it may be easier to just call ptr::write(iv, [0; 16])). Then they could be cast to &mut [u8; 16] references.
| &SslRef, | ||
| &mut [u8; 16], | ||
| *mut u8, | ||
| *mut ffi::EVP_CIPHER_CTX, |
There was a problem hiding this comment.
It's unfortunate that we have to pass these as raw pointers to users. How feasible is it to create safe bindings for these? If we do this then we'll have another task in our list for the 5.0 major bump
There was a problem hiding this comment.
callback configures hmac_ctx with an HMAC digest and key, and configures ctx for encryption or decryption, based on the mode.
So based on the docs the callback needs to configure the hmac and enc contexts.
For safe bindings I think we would need to expose a safe wrapper around EVP_CipherInit_ex, EVP_DecryptInit_ex and HMAC_Init_ex and also make it extensible enough for various uses that customers might want.
From what I can tell its not trivial and would probably be a separate task than this PR since its complex enough. That being said, I am not familiar with the 5.0 major bump initiative so can't comment on if its better to wait for safe bindings or if its ok to merge this now.
There was a problem hiding this comment.
To unblock this, we can make it pub unsafe fn set_ticket_key_callback_unsafe. The first unsafe, because we can't force the callback to do the correct initialization. The _unsafe suffix to leave room for a future set_ticket_key_callback that may have the right abstractions.
Add a wrapper for the
SSL_CTX_set_tlsext_ticket_key_cb, which allows consumers to configure the EVP_CIPHER_CTX and HMAC_CTX used for encrypting/decrypting session tickets.See https://docs.openssl.org/1.0.2/man3/SSL_CTX_set_tlsext_ticket_key_cb/ for more details.